FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect #3248

GalKepler · 2020-09-10T16:14:52Z

Resolves #3247 .

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

Resolves #3247 .

effigies · 2020-09-10T16:50:32Z

I think this may be a change in CLI...

  app.warn('Use of -fsl option in dwibiascorrect script is discouraged due to its strong dependence ' + \
           'on brain masking (specifically its inability to correct voxels outside of this mask).' + \
           'Use of the -ants option is recommended for quantitative DWI analyses.')

If this has changed, then we need to adjust our behavior based on the version of MRtrix3 installed.

effigies · 2020-09-11T14:00:25Z

If I'm reading it correctly, MRtrix3/mrtrix3#1449 changed from -ants and -fsl flags to ants and fsl subcommands, which means this behavior is new as of MRtrix3 3.0. @Lestropie is that correct?

Given that at least some MRtrix3 commands are Python, I wonder if there's a cleaner way to invoke these through a function API, rather than subprocesses. Though that also may have changed.

GalKepler · 2020-09-13T11:10:49Z

@effigies I think you're right, but I wonder whether to create a version-specific inputspec, or simply change the relevant trait in accordance with the user's Mrtrix3's version?
I've also posted a message in Mrtrix3's forum to try and verify the exact version in which the change occurred.

Lestropie · 2020-09-13T23:37:53Z

MRtrix3/mrtrix3#1449 changed from -ants and -fsl flags to ants and fsl subcommands, which means this behavior is new as of MRtrix3 3.0.

Correct; they changed from command-line options to using the MRtrix3 algorithm submodule (essentially subcommands), as this is more consistent with other MRtrix3 Python scripts. This also means that the algorithm name needs to be the second command-line argument, whereas previously the command-line option was not positional.

Given that at least some MRtrix3 commands are Python, I wonder if there's a cleaner way to invoke these through a function API, rather than subprocesses. Though that also may have changed.

Given the degree to which the MRtrix3 Python API is custom and very much nonstandard, I suspect not; at least not without essentially duplicating the functionality of the dwibiascorrect wrapper script. The majority of MRtrix3 is C++ terminal commands, so when Python scripts started to get added they were designed around the same philosophy.

I've also posted a message in Mrtrix3's forum to try and verify the exact version in which the change occurred.

The change is from 3.0_RC3 to 3.0.0. There's a couple of other tags (3.0_RC3_latest, 3.0_RC3_last, 3.0_RC4) that were not actually intended for public use. The relevant change is present in 3.0_RC4; but I'd advise avoiding that tag anyway, it was somewhat erroneous to include that tag at all on the main repo.

@effigies

Terminal warning issued to user was copy&pasted from 3.0_RC3, but not properly modified to reflect the changes to the dwibiascorrect interface introduced in 3.0.0. As observed in nipy/nipype#3248 by @effigies.

Lestropie · 2020-09-14T03:57:55Z

Also MRtrix3/mrtrix3#2171 may have contributed to the confusion; sorry about that.

effigies

Okay, then I think we want to add:

class DWIBiasCorrect(MRTrix3Base):
    ...
    def _format_arg(self, name, trait_spec, value):
        if name in ('use_ants', 'use_fsl'):
            ver = self.version
            # Changed in version 3.0, after release candidates
            if ver is not None and (ver[0] < '3' or ver.startswith("3.0_RC")):
                return f"-{trait_spec.argstr}"
        super()._format_arg(name, trait_spec, value)

And we need to update the doctest to say dwibiascorrect ants dwi.mif dwi_biascorr.mif.

nipype/interfaces/mrtrix3/preprocess.py

.zenodo.json

effigies · 2020-10-22T01:58:48Z

@GalBenZvi Do you have time to finish this one off? I think we're probably due for a release soon, and it would be nice to have this in.

GalKepler · 2020-10-22T07:44:05Z

@effigies I'm sorry, I was under the impression that it was finished, could you maybe explain to me what do you want me to do? Obviously I would love to contribute.

effigies · 2020-10-22T09:52:35Z

Sure, my previous review had a number of suggestions. If that doesn't seem actionable, I can send a PR against your branch with my suggested changes.

GalKepler · 2020-10-23T06:10:00Z

Sure, I'll do my best to get to it as soon as possible. Sorry for the delay.

Co-authored-by: Chris Markiewicz <[email protected]>

nipype/interfaces/mrtrix3/preprocess.py

codecov · 2020-11-08T14:22:11Z

Codecov Report

Merging #3248 (68b3003) into master (3976524) will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3248      +/-   ##
==========================================
- Coverage   65.02%   64.98%   -0.04%     
==========================================
  Files         302      302              
  Lines       39941    39947       +6     
  Branches     5281     5283       +2     
==========================================
- Hits        25973    25961      -12     
- Misses      12902    12914      +12     
- Partials     1066     1072       +6

Flag	Coverage Δ
unittests	`64.91% <66.66%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
nipype/interfaces/mrtrix3/preprocess.py	`85.29% <66.66%> (-1.17%)`	⬇️
nipype/pipeline/plugins/legacymultiproc.py	`65.68% <0.00%> (-5.40%)`	⬇️
nipype/pipeline/plugins/base.py	`58.08% <0.00%> (-1.65%)`	⬇️
nipype/interfaces/mrtrix3/base.py	`59.64% <0.00%> (+1.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3976524...68b3003. Read the comment docs.

effigies · 2020-11-08T14:52:07Z

Okay, this looks all set. Thanks for this addition!

GalKepler added 2 commits September 10, 2020 19:10

Removed "-" from algorithm specification

5d1c1e9

Resolves #3247 .

Update .zenodo.json

42544d0

Lestropie mentioned this pull request Sep 14, 2020

dwibiascorrect fsl: Revise warning MRtrix3/mrtrix3#2171

Merged

effigies reviewed Sep 14, 2020

View reviewed changes

nipype/interfaces/mrtrix3/preprocess.py Show resolved Hide resolved

nipype/interfaces/mrtrix3/preprocess.py Show resolved Hide resolved

.zenodo.json Outdated Show resolved Hide resolved

GalKepler and others added 4 commits October 26, 2020 11:59

Update nipype/interfaces/mrtrix3/preprocess.py

0021bd3

Co-authored-by: Chris Markiewicz <[email protected]>

Update nipype/interfaces/mrtrix3/preprocess.py

524482e

Co-authored-by: Chris Markiewicz <[email protected]>

Update .zenodo.json

613d647

Co-authored-by: Chris Markiewicz <[email protected]>

Edited file according to Chris's requests

a8a4c7f

effigies reviewed Oct 27, 2020

View reviewed changes

nipype/interfaces/mrtrix3/preprocess.py Outdated Show resolved Hide resolved

Update nipype/interfaces/mrtrix3/preprocess.py

5ddcc0d

effigies changed the title ~~FIX: Removed "-" from algorithm specification~~ FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect Oct 27, 2020

effigies added 2 commits November 8, 2020 09:00

TEST: make specs

73e98d2

Merge remote-tracking branch 'upstream/master' into patch-1

68b3003

effigies merged commit b356ad1 into nipy:master Nov 8, 2020

effigies added this to the 1.6.0 milestone May 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect #3248

FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect #3248

GalKepler commented Sep 10, 2020

effigies commented Sep 10, 2020

effigies commented Sep 11, 2020 •

edited

Loading

GalKepler commented Sep 13, 2020

Lestropie commented Sep 13, 2020

Lestropie commented Sep 14, 2020

effigies left a comment

effigies commented Oct 22, 2020

GalKepler commented Oct 22, 2020

effigies commented Oct 22, 2020

GalKepler commented Oct 23, 2020

codecov bot commented Nov 8, 2020 •

edited

Loading

effigies commented Nov 8, 2020

FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect #3248

FIX: Handle changes in CLI structure of mrtrix3.DWIBiasCorrect #3248

Conversation

GalKepler commented Sep 10, 2020

effigies commented Sep 10, 2020

effigies commented Sep 11, 2020 • edited Loading

GalKepler commented Sep 13, 2020

Lestropie commented Sep 13, 2020

Lestropie commented Sep 14, 2020

effigies left a comment

Choose a reason for hiding this comment

effigies commented Oct 22, 2020

GalKepler commented Oct 22, 2020

effigies commented Oct 22, 2020

GalKepler commented Oct 23, 2020

codecov bot commented Nov 8, 2020 • edited Loading

Codecov Report

effigies commented Nov 8, 2020

effigies commented Sep 11, 2020 •

edited

Loading

codecov bot commented Nov 8, 2020 •

edited

Loading